Allow casting a Mixed column into a concrete type #6777

radeusgd · 2023-05-19T11:19:12Z

Pull Request Description

Follow-up of #6711

Closes #6838

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

GregoryTravis · 2023-05-24T19:01:43Z

std-bits/table/src/main/java/org/enso/table/data/column/operation/cast/CastProblemBuilder.java

+    failedConversionsCount++;
+  }
+
+  public int getFailedConversionsCount() {


Is there a way for the user to know which values failed to convert?

Currently, not directly.

They are replaced with Nothing in the resulting table (although there can be existing Nothing values in there, so it is not unambiguous).

Do you think there should be? I was afraid that the error message may get too long.

But maybe we can gather the first 3... or 10? values that failed to convert and display these. I guess that might be helpful. I can add it. Shall I?

What do you think a decent limit would be? IMO 3 sounds good as it shows a few examples. Anything more will make the error message too long anyway.

I'm not really sure; a few might be useful, because that might be enough to fix the problems. On the other hand, the user might have to iterate many times to find all the errors, if there are a lot of problems. And I would hope that at some point we have a way to implement this that doesn't require that we re-implement it for every Column method. :)

I think this is fine for now, until we have a more general plan.

And I would hope that at some point we have a way to implement this that doesn't require that we re-implement it for every Column method. :)

I think error handling is very usage-specific, so there is no one-fits-all solution.

In fact, I'm leaning towards creating tailored ones for each method (group), as I first attempted to create a generic Problem_Builder but it turns out it gets unnecessarily complex if we try to solve all kinds of scenarios using one tool.

radeusgd · 2023-05-24T19:08:09Z

test/Table_Tests/src/Common_Table_Operations/Conversion_Spec.enso

+                if setup.test_selection.fixed_length_text_columns then
+                    c3_2 = t2.cast "super-mix" (Value_Type.Char size=2) . at "super-mix"
+                    c3_2.value_type . should_equal (Value_Type.Char size=2)
+                    c3.to_vector . should_equal ["1.", "2.", "3", "-4", "2.", "X", "20", "20", "12", "{{", "Tr", Nothing, "Tr"]
+                    w3_2 = Problems.expect_warning Conversion_Failure c3_2
+                    w3_2.affected_rows_count . should_equal 10


I'm wondering if the 'trimming' of string representations should be noted as Conversion_Failure or should it get some separate error type? What do you think?

(my motivation is that we could get a bit clearer error message for this case)

Maybe we should add a second variant to Conversion_Failure?

type Conversion_Failure Unfit_For_Target_Type ... # <- existing one Text_Representation_Too_long # <- for text trimming case

I think we should report a problem -- it's not like other methods, where the user is explicitly requesting trimming/truncation, so it might be a surprise and hidden data loss is bad. Text_Representation_Too_long is a good error to know about.

It is reported (unless text->text in which case trimming is expected).

I'm mostly wondering if we should have a separate error type for this case or a single error for all conversion issues.

I guess the 1 error with 2 constructors may be a decent compromise.

enso-bot · 2023-05-25T09:32:46Z

Radosław Waśko reports a new STANDUP for yesterday (2023-05-24):

Progress: Catching up after timeoff. Updating my env after GraalVM upgrade. Reviews. Meeting about imports/exports. Finished Mixed casts implementation and prepared the PR. It should be finished by 2023-05-26.

Next Day: Next day I will be working on the #6410 task. Catch up with bookclubs. Get the Mixed types PR merged. Start work on add_row_numbers.

jdunkerley

Generally looks good a couple of nit comments.

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Errors.enso

std-bits/table/src/main/java/org/enso/table/data/column/builder/object/NumericBuilder.java

...table/src/main/java/org/enso/table/data/column/operation/cast/ToBooleanStorageConverter.java

...ts/table/src/main/java/org/enso/table/data/column/operation/cast/ToDateStorageConverter.java

…nclude example values.

* develop: Allow casting a Mixed column into a concrete type (#6777) Stop graph editing when in full-screen visualization mode (#6844) Handle `show-dashboard` event (#6837) Fix some dashboard issues (#6668) Fix JWT leak (#6815) Fix "set username" screen (#6824) Fallback to opened date when ordering projects (#6814) Various test improvements to increase coverage and speed things up (#6820) do not activate nested dropdowns together (#6830) Clearly select single specialization with enum dispatch pattern (#6819) Prevent incorrect application of list widget on incompatible expressions (#6771)

…le-6756-6804 * develop: (22 commits) Coalesce graph editor view invalidations (#6786) Append warnings extracted before tail call execution (#6849) Detect and override hooks of the same kind (#6842) Dynamic app resampling and better performance measurements. (#6595) Show spinner when opening/creating a project, take #2 (#6827) Infrastructure for testing inter project imports and exports (#6840) Only initialise visualisation chooser if it is used. (#6758) Allow casting a Mixed column into a concrete type (#6777) Stop graph editing when in full-screen visualization mode (#6844) Handle `show-dashboard` event (#6837) Fix some dashboard issues (#6668) Fix JWT leak (#6815) Fix "set username" screen (#6824) Fallback to opened date when ordering projects (#6814) Various test improvements to increase coverage and speed things up (#6820) do not activate nested dropdowns together (#6830) Clearly select single specialization with enum dispatch pattern (#6819) Prevent incorrect application of list widget on incompatible expressions (#6771) Update GraalVM to 22.3.1 JDK17 (#6750) Import/export syntax error have more specific messages (#6808) ...

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label May 19, 2023

radeusgd self-assigned this May 19, 2023

radeusgd force-pushed the wip/radeusgd/simple-parse branch from 6296d34 to 3b6fcd2 Compare May 19, 2023 16:20

radeusgd force-pushed the wip/radeusgd/mixed-cast branch from 9eb8c12 to 1c8551d Compare May 19, 2023 22:08

Base automatically changed from wip/radeusgd/simple-parse to develop May 19, 2023 22:11

radeusgd force-pushed the wip/radeusgd/mixed-cast branch from 1c8551d to 5938150 Compare May 24, 2023 17:48

radeusgd marked this pull request as ready for review May 24, 2023 18:39

radeusgd requested review from jdunkerley and GregoryTravis as code owners May 24, 2023 18:39

GregoryTravis approved these changes May 24, 2023

View reviewed changes

radeusgd commented May 24, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/mixed-cast branch 2 times, most recently from 31a15d9 to 24924f3 Compare May 25, 2023 18:21

jdunkerley approved these changes May 26, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/mixed-cast branch from 24924f3 to 5e4e87d Compare May 26, 2023 09:04

radeusgd added 14 commits May 26, 2023 13:26

spec for Mixed->X casts

950d162

comments

ecb3572

Remove a redundant class

deba626

rewriting how casting strategy is laid out

1f36665

try a better message for No Common Types error

daf8653

move

11d4f86

Implement conversions in new format.

b748fad

switch to new cast. remove old.

cf369ce

fixes

7b7cadc

refactor: cleanup

8d2b975

fixes

a80f1c3

imports

16221c3

javafmt

3010654

prepare to text cast for fixed length texts

f726d9d

radeusgd added 6 commits May 26, 2023 13:26

some docs, warn on string truncation

cfd703a

Improve error reporting: separate variant for Text representations, i…

036508e

…nclude example values.

CR: update wording in comments

fa49846

CR: No_Common_Type phrasing

2f28a2c

nicer message in Conversion_Failure error

763114e

split NumericBuilder into proper subclasses

c7ca8e7

radeusgd force-pushed the wip/radeusgd/mixed-cast branch from 5e4e87d to c7ca8e7 Compare May 26, 2023 12:30

radeusgd added 2 commits May 26, 2023 14:31

CR: unnecessary empty ctor

6321fef

javafmt

06f4268

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label May 26, 2023

mergify bot merged commit c3e771c into develop May 26, 2023

mergify bot deleted the wip/radeusgd/mixed-cast branch May 26, 2023 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow casting a Mixed column into a concrete type #6777

Allow casting a Mixed column into a concrete type #6777

radeusgd commented May 19, 2023 •

edited

Loading

GregoryTravis May 24, 2023

radeusgd May 24, 2023

radeusgd May 24, 2023

GregoryTravis May 24, 2023

GregoryTravis May 24, 2023

radeusgd May 25, 2023

radeusgd May 24, 2023 •

edited

Loading

radeusgd May 24, 2023

GregoryTravis May 24, 2023

radeusgd May 25, 2023

enso-bot bot commented May 25, 2023

jdunkerley left a comment

Allow casting a Mixed column into a concrete type #6777

Allow casting a Mixed column into a concrete type #6777

Conversation

radeusgd commented May 19, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enso-bot bot commented May 25, 2023

jdunkerley left a comment

Choose a reason for hiding this comment

radeusgd commented May 19, 2023 •

edited

Loading

radeusgd May 24, 2023 •

edited

Loading